London | 26-ITP-January | Eugenie Ahangama | Sprint 1 | Data Groups#998
London | 26-ITP-January | Eugenie Ahangama | Sprint 1 | Data Groups#998Eugenie-A wants to merge 15 commits intoCodeYourFuture:mainfrom
Conversation
Eugenie-A
added
🏕 Priority Mandatory
| // Return null if no valid numbers remain | ||
| if (nums.length === 0) return null; | ||
|
|
||
| const sorted = [...nums].sort((a, b) => a - b); |
There was a problem hiding this comment.
Why make a copy of nums before sorting?
There was a problem hiding this comment.
I used [...nums] because I wanted to avoid mutating the input array list. However, since nums is already created by filter, it's a brand new array and not a reference to list. So sorting nums directly won't affect the original input, making the spread unnecessary. I'll remove it.
cjyuan
added
Reviewed
Eugenie-A
added
Needs Review
| test("given an array with no duplicates, it returns a copy of the original array", () => { | ||
| const input = [1, 2, 3]; | ||
| const result = dedupe(input); | ||
| // Check the contents are the same | ||
| expect(result).toEqual(input); | ||
| // Check it's a different array, not the same reference | ||
| expect(result).not.toBe(input); | ||
| }); |
There was a problem hiding this comment.
Your function is correct, but currently there is a chance that an incorrectly implemented function can pass this test. Can you figure out why?
There was a problem hiding this comment.
A function that just returns a copy without deduplicating (e.g. return [...arr]) would still pass since [1, 2, 3] has no duplicates.
I've added an extra check using an array with duplicates [1, 1, 2, 3] to make the test more robust.
cjyuan
added
Reviewed
Eugenie-A
added
Needs Review
| // Check that a duplicate input is correctly deduped in the result | ||
| expect(dedupe([1, 1, 2, 3])).toEqual([1, 2, 3]); |
There was a problem hiding this comment.
The case of "array with duplicates" is checked else where.
I was referring to the specific case of "array with no duplicates".
Hint: Is it possible that result and input can have the same wrong content?
There was a problem hiding this comment.
I see, if dedupe mutated input, then toEqual(input) would still pass since both would have the same wrong content. I've fixed it by comparing result against a hardcoded [1, 2, 3] instead.
cjyuan
added
Reviewed
Eugenie-A
added
Needs Review
|
All good now. Well done! |
cjyuan
added
Complete
|
Closing PR because the January ITP run has finished. Feel free to re-open if you're still working on it. |
3 participants
Learners, PR Template
Self checklist
Changelist
calculateMedianimplementation inmedian.jsdedupe.jsand wrote tests indedupe.test.jsfindMaxinmax.jsand wrote tests inmax.test.jssum.jsand wrote tests insum.test.jsincludes.jsfrom aforloop to afor...of loopand added comments to both filesQuestions
What makes a good test? What makes a bad test?